Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement Toggler widget for iced_native #535

Merged
merged 14 commits into from
Jun 3, 2021
Merged

Conversation

Kaiden42
Copy link
Contributor

@Kaiden42 Kaiden42 commented Sep 19, 2020

This PR implements the Toggler widget mentioned in #528 . The Toggler widget works similar to the Checkbox widget.

togglers

I am open to suggestions for improvement. Currently iced_web is not supported, but maybe I can check it out in the near future.

Closes #528.

Copy link
Contributor

@clarkmoody clarkmoody left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a great start!

What I would like to see:

  • More flexibility when it comes to the label. Right now, we've got a String on the left side of the toggler. I'd like to see a generic impl Into<Element<'a, Message>> as the label, with the option of placing it on the right or left side.
  • Maybe instead of the previous, we simply add a with_toggler helper method that joins the elements to a toggler, and the toggler itself becomes a standalone widget without the label.
  • Either an example program showing off the toggler, or an update to the tour example with toggler in there

@Kaiden42
Copy link
Contributor Author

Thank you very much for reviewing my PR.

  • I agree, the label needs more flexibility. However, I don't think taking an impl Into<Element<'a, Message>> instead of a String would be the right choice here. I can't imagine a valid usecase for e.g. a Slider to be the label for a Toggler. This doesn't sound to me. I could add a function for specifying the alignment of the label instead.

  • As mentioned in the previous point, only a Text would be a useful element for a label. Maybe making the label optional for a stand alone Toggler would be more useful instead.

  • Alright! I could update the tour and / or maybe the styling example(s), showing how to use (and style) the Toggler widget.

@clarkmoody
Copy link
Contributor

You might want a small SVG image to be the label instead of a String of text.

If you want to bold a portion of the label, you'll need more than one Text joined with a container.

I think it's more useful, like you say, to avoid the label altogether, or use Option<String>.

@hecrj
Copy link
Member

hecrj commented Sep 22, 2020

Let's keep things simple!

I think it's fine to satisfy only a subset of potential use cases. I prefer to tackle a small amount of specific use cases directly instead of a flexible API that can be used wrong.

That said, I believe removing the label would simplify the widget considerably. We should probably come up with a Label widget of some sort eventually.

@Kaiden42
Copy link
Contributor Author

I've now made the label optional and added a function to specify the alignment of the label. Specifying the alignment of the label can be usefull when using a 'right to left' language instead of a 'left to right' language.

In addition, I've also added the Toggler to the styling and tour example.

Currently the default width of the widget is Length::Fill. This is needed for a good looking alignment with a lable especially when using multiple togglers in a Column. However, when avoiding the label Length::Shrink would be a better default value. Should I add some logic for choosing the default value of the width to Length::Fill when using a label and Length::Shink when not or would this just be to confusing?

@hecrj hecrj added the feature New feature or request label Sep 30, 2020
@Kaiden42
Copy link
Contributor Author

Kaiden42 commented Oct 3, 2020

I've now added the Toggler widget to iced_web as well. However, I'm unhappy about the css styling. Maybe someone with more experience in web developement could take a look into it.

@hecrj hecrj added this to the 0.3.0 milestone Dec 18, 2020
@hecrj hecrj modified the milestones: 0.3.0, 0.4.0 Mar 31, 2021
Copy link
Member

@hecrj hecrj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have rebased on top of master and made a couple of small changes.

I think we can finally merge this! Thank you @Kaiden42! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Toggler Widget
3 participants